New Card and FormFieldSet components
#4204
Conversation
amcclain
changed the title
Card and FormFieldSet components
|
I am happy to look myself as soon as I have a chance, but I would like to see if we could merge this first step into |
amcclain
left a comment
amcclain
left a comment
There was a problem hiding this comment.
Will continue this review - haven't looked carefully at FormFieldSet yet but wanted to get this round of comments posted.
| import {bindable, makeObservable} from '@xh/hoist/mobx'; | ||
| import {isNil} from 'lodash'; | ||
|
|
||
| export interface CardConfig { |
There was a problem hiding this comment.
Realize that this is our more common standard - but still asking the question - would CardModelConfig be incrementally more clear? I feel like our comp props vs model config story is already a common source of confusion - we're using Config diligently to indicate "model config object" vs "comp props", but any harm in the extra word to be even more explicit?
| fromContext: false, | ||
| publishMode: 'limited', | ||
| createDefault: true | ||
| }), |
There was a problem hiding this comment.
So I mentioned this w.r.t. Panel + PanelModel, which is a common source of annoyance for me when looking to author app-level components that ultimately render a Panel and want to take and pass along all (or at least most) Panel props.
Those app level components almost always have some app-level model that they are actually using/creating. This means that if I want to have a custom panel-based comp - say TradeDetailPanel + TradeDetailModel and I want my component to support resizing, I have to do some workaround like add a custom panelModel prop to my app comp that I relay to the underlying Panel as its model.
It's not the worst thing, but I do think we somewhat overloaded the component's primary "model" here in a way that makes some usage patterns more difficult. The extra friction of handling that props makes it more likely that apps have components named FooPanel that don't take expected props, or get an extra layer of nesting to support resizability (if the comp author doesn't bother take the approach above).
Rather than doubling down on that, could we consider something like a layoutModel or renderModel prop to accept one of these, or a config for one? I'd like to think about how this might play with e.g. a proper Hoist dialog or drawer component as well, and ultimately consider a change to Panel if we think this is a better option for these common container + building-block comps.
| } from '@xh/hoist/core'; | ||
| import {collapseToggleButton as desktopCollapseToggleButtonImpl} from '@xh/hoist/dynamics/desktop'; | ||
| import {collapseToggleButton as mobileCollapseToggleButtonImpl} from '@xh/hoist/dynamics/mobile'; | ||
| import {tooltip as bpTooltip} from '@xh/hoist/kit/blueprint'; |
There was a problem hiding this comment.
I checked and it looks like currently we only have a single import from @xh/hoist/kit/blueprint that's outside of the top-level desktop packages (desktop/admin/inspector). That's utils/impl/MenuItems.ts.
We should carefully consider if we're OK with BP imports getting into mobile code, and review how the bundles are created to see if this drags in more than we expect for mobile app builds. Or we could go the other way and stop being so hesitant to use BP in a mobile context, but until now (other than that one perhaps unfortunate exception) we've kept them isolated.
| : desktopCollapseToggleButtonImpl; | ||
|
|
||
| if (collapsed) { | ||
| classes.push('xh-card--collapsed'); |
| switch (renderMode) { | ||
| case 'always': | ||
| content = items; | ||
| classes.push('xh-card--render-mode-always'); |
There was a problem hiding this comment.
How do we anticipate these CSS classes being useful? I wasn't sure why you would be targeting these variations on a CSS level. Do we do this with other renderMode enabled containers?
| break; | ||
| } | ||
|
|
||
| return fieldset({ |
There was a problem hiding this comment.
Worth a comment somewhere in this component re. the use of HTML fieldset and its legend element, below?
| /** | ||
| * A convenience button to toggle a Card's expand / collapse state. | ||
| */ | ||
| export const [CollapseToggleButton, collapseToggleButton] = |
There was a problem hiding this comment.
The name of this component makes it sounds more general than it is. Is it being in the card package enough, do we think? Any reason not to do CardCollapseToggleButton or CardExpandCollapseButton?
Hoist P/R Checklist
Pull request authors: Review and check off the below. Items that do not apply can also be
checked off to indicate they have been considered. If unclear if a step is relevant, please leave
unchecked and note in comments.
developbranch as of last change.breaking-changelabel + CHANGELOG if so.If your change is still a WIP, please use the "Create draft pull request" option in the split
button below to indicate it is not ready yet for a final review.